Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: TraceQL metrics: average over time #4073

Merged
merged 31 commits into from
Oct 31, 2024
Merged

feat: TraceQL metrics: average over time #4073

merged 31 commits into from
Oct 31, 2024

Conversation

javiermolinar
Copy link
Contributor

@javiermolinar javiermolinar commented Sep 12, 2024 •
edited
Loading

What this PR does:
It implements the avg_over_time aggregation function

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@javiermolinar javiermolinar marked this pull request as ready for review September 12, 2024 14:59
@mdisibio
Copy link
Contributor

Not sure if I'm reading this right, but it looks like it's computing the average at each level. I was expecting raw mode to return series of sums/counts up the frontend, and then it computes the final average. Without that, it seems to be computing average of averages inside generators and queriers. We can discuss more details like the algorithm, but let's clear the overall approach first.

@javiermolinar javiermolinar marked this pull request as draft September 17, 2024 09:13
@javiermolinar javiermolinar marked this pull request as ready for review October 17, 2024 15:27
mdisibio
mdisibio reviewed Oct 18, 2024
View reviewed changes
pkg/traceql/engine_metrics_average.go Outdated Show resolved Hide resolved
pkg/traceql/engine_metrics_average.go Outdated Show resolved Hide resolved
mdisibio
mdisibio reviewed Oct 18, 2024
View reviewed changes
pkg/traceql/engine_metrics_average.go Outdated Show resolved Hide resolved
pkg/traceql/engine_metrics_average.go Show resolved Hide resolved
pkg/traceql/engine_metrics_average.go Outdated Show resolved Hide resolved
pkg/traceql/engine_metrics_average.go Show resolved Hide resolved
pkg/traceql/engine_metrics_average.go Outdated Show resolved Hide resolved
pkg/traceql/engine_metrics_average.go Outdated Show resolved Hide resolved
pkg/traceql/engine_metrics_average.go Outdated Show resolved Hide resolved
knylander-grafana
knylander-grafana reviewed Oct 21, 2024
View reviewed changes
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding doc! Approving the doc, with two minor suggestions. Please be aware that this PR updates the metrics queries doc: #4128

mdisibio
mdisibio approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the last round of changes. This is looking great.

@knylander-grafana knylander-grafana added the type/docs Improvements or additions to documentation label Oct 31, 2024
knylander-grafana
knylander-grafana approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the doc changes. Thank you for updating!

docs/sources/tempo/traceql/metrics-queries/functions.md Outdated Show resolved Hide resolved
docs/sources/tempo/traceql/metrics-queries/functions.md Outdated Show resolved Hide resolved
docs/sources/tempo/traceql/metrics-queries/functions.md Outdated Show resolved Hide resolved
docs/sources/tempo/traceql/metrics-queries/functions.md Outdated Show resolved Hide resolved
docs/sources/tempo/traceql/metrics-queries/_index.md Outdated Show resolved Hide resolved
@javiermolinar javiermolinar merged commit 61fa0a5 into grafana:main Oct 31, 2024
17 checks passed
@javiermolinar javiermolinar deleted the avg-over-time branch October 31, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knylander-grafana knylander-grafana knylander-grafana approved these changes

@mdisibio mdisibio mdisibio approved these changes

@joe-elliott joe-elliott Awaiting requested review from joe-elliott joe-elliott is a code owner

@annanay25 annanay25 Awaiting requested review from annanay25

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@yvrhdn yvrhdn Awaiting requested review from yvrhdn yvrhdn is a code owner

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

@electron0zero electron0zero Awaiting requested review from electron0zero electron0zero is a code owner

@ie-pham ie-pham Awaiting requested review from ie-pham ie-pham is a code owner

@stoewer stoewer Awaiting requested review from stoewer stoewer is a code owner

Assignees
No one assigned
Labels
type/docs Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@javiermolinar @mdisibio @knylander-grafana